fix(txn): make the SSI commit-time pivot check atomic with the commit decision (#136) - #151

The defect

A write skew commits under DB_TXN_SNAPSHOT when the second transaction's
write lands while the first is inside DB_TXN->commit. Both commits return 0
and the stored state has no serial order. Reported in #136 with a complete
reproducer and a correct root-cause analysis.

Two halves of one race:

(a) The commit-side check was not atomic with the status transition.
__txn_commit read both pivot flags under TXN_SYSTEM_LOCK, then released it —
but td->status stays TXN_RUNNING until __txn_end publishes
TXN_COMMITTED, far later, past cursor close, lease checks and the log write.
Throughout that span T1 looked like a running transaction whose pivot check was
still ahead of it. The in-code comment claiming the test and the commit decision
are atomic was true only of the two flag reads, not of the decision.

(b) The writer-side "reader will abort itself" optimization trusted that
stale status. In __lock_get_internal, T2 met T1's SIREAD marker, saw
status == TXN_RUNNING and TXN_DTL_WCONF already set, and concluded T1
would abort at a later pivot check. So it skipped the branch that would have
rejected T2 for its own existing TXN_DTL_RCONF, did not set WCONF on itself,
and only added RCONF to T1 — a transaction that had already passed its one and
only pivot check. T1 ended up holding both pivot flags with nobody left to look
at them; T2 held only RCONF; both committed.

Design chosen: option 1 — publish a "checked/committing" state under the same mutex as the check

A passing pivot check now publishes TXN_DTL_SICHECKED on the detail in the
same TXN_SYSTEM_LOCK critical section as the check itself. The writer-side
fate tests ask "can the peer still resolve this edge?" through a new
TXN_SI_PAST_CHECK(td) predicate instead of status == TXN_COMMITTED, so a
committing-but-not-yet-committed peer is treated like a committed one and the
writer resolves the edge itself (DB_SNAPSHOT_UNSAFE).

Why the alternatives are worse:

• Option 2, re-check after the point of no return — not correct. The
  "last moment" before status = TXN_COMMITTED in __txn_end is after
  __txn_regop_log/log flush and after __lock_vec(DB_LOCK_PUT_READ) released
  the read locks. Aborting there is not available: __txn_end is documented as
  unable to return an error and panics on failure, and the commit record is
  already durable, so a replica or a recovery pass would already have accepted
  the commit. It also cannot use goto err (which calls __txn_abort) because
  the transaction's locks are gone. The task brief anticipated this, and reading
  the code confirms it: the window is not abortable, so the state must be
  published at the decision, not re-examined after it.
• Making the writer test status != TXN_RUNNING — wrong direction: it would
  treat an aborted peer as "past check", turning every edge into a doomed
  transaction into a spurious abort of the writer. An aborted transaction's reads
  never committed, so an edge into it is not a conflict at all. Hence
  TXN_SI_PAST_CHECK tests TXN_COMMITTED || (TXN_RUNNING && SICHECKED).
• Atomic flags word / wider locking — the existing analysis note already
  rejected converting TXN_DETAIL.flags to an atomic: it carries many non-SSI
  bits and atomics alone would not make the decision atomic. Both sides already
  serialize on TXN_SYSTEM_LOCK; the missing ingredient was state, not more
  mutual exclusion.
• Abort the pivot the moment the second edge forms (no commit-time check at
  all) — arguably the better long-term shape, but a much larger behavioral change
  than a bug fix should carry.

Also fixed the same staleness at the mirror site in mpool
(__memp_si_rwconflict), which had the identical wtd->status == TXN_COMMITTED
test. The reported reproducer does not reach it, but it is the same defect one
mechanism over — the lazy fix is the one predicate used by both callers.

Constraints honoured

• No layout/ABI/format change. TXN_DTL_SICHECKED is the spare bit 0x80
  in TXN_DETAIL's existing u_int32_t flags word (bits through 0x40 were
  taken). Verified by compiling the struct both ways:

  	master	this branch
  sizeof(TXN_DETAIL)	344	344
  offsetof(flags)	140	140
  offsetof(links)	144	144
  offsetof(slots)	312	312
  sizeof(DB_TXNREGION)	208	208

  No on-disk, log or region-layout change; no public ABI change. No new message
  IDs (no new user-visible strings), so s_message_id was not needed.

• Lock ordering unchanged. No new mutex and no new nesting: both sides
  already took TXN_SYSTEM_LOCK around their flag access. The commit critical
  section grows by exactly one F_SET on a word already in cache.

• Existing correct behaviors preserved: control still yields
  DB_SNAPSHOT_CONFLICT, late still yields DB_SNAPSHOT_UNSAFE.

Proof the race is closed

The reporter's own reproducer, built against this branch, on both the debug
(--enable-debug --enable-diagnostic) and the release (CFLAGS=-O2) trees:

It is a race, so it was run repeatedly rather than once:

• debug build: 60 + 40 = 100 trigger runs, 100 control, 100 late -> 300/300
  serializable, 0 violations. Every trigger run gave DB_SNAPSHOT_UNSAFE;
  every control gave DB_SNAPSHOT_CONFLICT.
• release -O2 build: 25 runs per mode -> 75/75 serializable.
• On master the trigger reproduced on the first attempt, every attempt.

The isolation tier, expectations cleared

expect_fail is now clear on write_skew_trigger and
write_skew_samebtree_trigger (and the README table/exit-status section
updated), so the tier is a plain regression gate — any violation is a new bug.

9 scenario(s) run, 0 unexpected outcome(s)

Run 4x in full (each racy scenario doing 40 attempts) plus 5 extra runs of just
the two trigger scenarios: ~700 racy attempts, 0 non-serializable histories.

Both #136 shapes now show T2 read alice=1; put bob=0 -> DB_SNAPSHOT_UNSAFE.

On the reporter's "separate defect"

Confirmed not a second bug, with two independent constructions: the tier's
write_skew_samebtree_control (512-byte pages + filler keys, 33 leaf pages
verified via DB->stat→bt_leaf_pg, alice=min key, bob=max key) and the
reporter's own two-one-page-DB shape both return DB_SNAPSHOT_CONFLICT at
control timing and only fail at trigger timing. So the different-pages shape has
the same single root cause. The reporter never published their same-btree
variant, so their exact shape is unverified — with two records at the default
page size those sit on one page, a case they themselves report behaves
correctly. The samebtree_control PASS expectation is kept live so a real
page-granularity hole would surface there.

Regression matrix

ASan runs used LD_PRELOAD=$(cc -print-file-name=libasan.so) ASAN_OPTIONS=detect_leaks=0 as required for the instrumented libdb_tcl.

Builds: --enable-debug --enable-diagnostic --enable-test,
--enable-diagnostic, release CFLAGS=-O2, --enable-debug --enable-dst,
ASan, and meson setup build && ninja -C build — all clean, 0 errors.

Measured commit-path cost

The fix adds one F_SET inside an already-held critical section, on the
SSI-only path (F_ISSET(txn, TXN_SNAPSHOT_SAFE)); non-SSI commits execute
identical code. test/bench/ssi_abort_bench, A/B against master built the same
way (-O2), alternating runs:

Honest reading: this bench cannot resolve the cost. Run-to-run spread within
one config is ±2x (e.g. master alone ranged 1534–3066 txn/s at hot=4096), which
swamps any effect of a single store; the fix is ahead in two of three configs
and behind in one, which is noise, not signal. What is structural: the
critical section is not widened by any lock acquisition, loop or I/O — one bit
set on a word the same line already read — and the SSI abort rate is unchanged
(12.3–13.0% both sides at hot=16), so the fix is not converting benign
schedules into aborts at scale. It only aborts the writer in the specific
commit-window schedule that previously produced a non-serializable history.

Docs

• rfc/0003-ssi-serializable-snapshot-isolation.md — removed the libdb 5.3.34: DB_TXN_SNAPSHOT commits a write skew when the second write starts during the first commit #136 bullet
  from Known limitations (retitled to libdb 5.3.34: sequential read-only DB_TXN_SNAPSHOT transactions eventually cause DB_ENV->txn_begin to return ENOMEM #137–libdb 5.3.34: incomplete replication commit lock lists can violate client transaction isolation #140; the other items are left
  intact) and documented the delivered mechanism in Design.
• docs_src/api/c/txnbegin.md — removed the libdb 5.3.34: DB_TXN_SNAPSHOT commits a write skew when the second write starts during the first commit #136 half of the
  DB_TXN_SNAPSHOT known-limitation caveat, including the different-pages
  claim; the libdb 5.3.34: sequential read-only DB_TXN_SNAPSHOT transactions eventually cause DB_ENV->txn_begin to return ENOMEM #137/libdb 5.3.34: __txn_reap_si_details leaks MVCC mutex slots during snapshot cleanup #138 mutex-exhaustion caveat stays.
• test/isolation/README.md — expectation table, exit-status section.

Fixes #136

Supersedes #150, which was opened from a pre-rebase base. That branch is 3 commits stale and merging it would have deleted the 14 coverage-driver files added in #147 (-5304 lines); this branch is the same change rebased onto current master.